[톰캣 구현하기 - 3, 4단계] 히이로(문제웅) 미션 제출합니다. #455

MoonJeWoong · 2023-09-10T15:47:20Z

안녕하세요 도기, 히이로입니다!

원래는 토요일까지는 PR을 날려서 주말에 여유로우실 때 리뷰를 요청하고 싶었으나... 개인적인 일정들이 갑자기 많이 생겨서 주말이 다 지나서야 요청드리네요 😭
지난 번에 말씀해주신대로 전체적인 코드 구조를 개선하는데 집중했는데 도기가 보셨을 때 적절한 책임분리가 잘 되었다고 생각하시는지 궁금합니다. 이에 대해 의견 나눠보면 좋을 것 같아요!

항상 꼼꼼한 리뷰 감사드립니다. 이번에도 잘 부탁드려요! 🙇

kdkdhoho

안녕하세요 히이로! 3, 4단계 쉽지 않았을텐데 정말 고생 많으셨습니다.
이번 리뷰에는 코드 스타일, 변수명 등 코드 품질보다는 각 객체의 역할, 의존성, 구조 등을 중점적으로 봤어요.
주로 히이로의 의견이 궁금한 부분들을 리뷰로 남겨두었습니다.

감사합니다 :)

ps. 아 참 소나클라우드에서 제안하는 내용들 모두 꽤 괜찮더라구요? 리뷰 반영하시면서 소나 클라우드도 한번 참고하는걸 추천드립니다 ㅎㅎ

tomcat/src/main/java/nextstep/org/apache/catalina/connector/Connector.java

tomcat/src/main/java/nextstep/org/apache/coyote/http11/request/Http11Request.java

kdkdhoho · 2023-09-11T12:24:43Z

tomcat/src/main/java/nextstep/org/apache/coyote/http11/request/StartLine.java

@@ -14,14 +14,14 @@ public class StartLine {

    private String httpMethod;
    private String path;
-    private String httpVersion;
    private String queryString = null;


queryString을 따로 필드로 빼기보다는, path에 쿼리스트링도 포함하여 가지고 있다가, 필요로 할 때 따로 추출해서 반환하는 메서드가 하나 있으면 좋을 것 같아요.

저는 개인적으로 생각해봤을 때 path라는 변수에 쿼리스트링이 포함된 값이 저장되는게 어색하더라구요~ 혹시 Http정의에서 path는 queryString을 포함하는 개념일까요? 그런 부분이 있다면 다시 알려주시면 감사하겠습니당 🙇

mdn 문서에서는 path 변수와 함께 설명하기도 하고, 이 글이나, 저 글에서도 모두 "url 뒤에 붙는 문자열이다" 정도로만 설명하고, 따로 분리해서 생각하진 않네요.
이렇게 봐서는 path에 queryString을 포함하는 개념으로 보이긴 하네요.

사실 coyote/Request 객체에서도 MessageBytes queryMB = MessageBytes.newInstance();이라는 필드가 있고 이게 쿼리 파라미터를 가지는 필드같은데요. 오늘 강의 시간에 구구께서 객체지향 생활체조도 지키며 미션 진행하라는 말씀을 빌려서라도 함께 저장하는건 어떤가 다시 여쭤보고 싶습니다!

tomcat/src/main/java/nextstep/org/apache/catalina/servlet/LoginServlet.java

kdkdhoho · 2023-09-11T13:18:04Z

tomcat/src/main/java/nextstep/org/apache/coyote/http11/HttpUtil.java

 import java.util.Map;
+import java.util.Objects;
+import java.util.Optional;

 public class HttpUtil {


여기에 있는 메서드들, 모두 Http11Request 내부로 이동해도 좋을 것 같아요.
오히려, Http11Request가 이런 역할을 하는 것이 좀 더 적절하다고도 생각이 드네요 🧐

음 Http11Request 뿐만 아니라 Http11Processor를 비롯해 다른 서블릿에서도 사용될 수 있는 로직들이라 util로 분리해냈는데요, 어떤 부분에서 도기가 그렇게 생각하셨는지 좀 더 말씀해주셔도 좋을 것 같습니다 :)

이번 리뷰에서는 각 객체들의 역할을 중점적으로 보았는데요.

현재 Http11Processor는 createResponseBody, selectFirstContentTypeOrDefault를 사용하고 있네요.
그런데 이 두 메서드는 HttpResponse에서도 수행할 수 있다고 생각합니다.

preprocessRequestPath 이 메서드는, StartLine을 의존하는 Http11Request가 수행해도 충분하다고 생각돼요.

위 같은 이유로 말씀 드렸습니다.

그리고 사실, preprocessRequestPath의 로직이 AbstractServlet의 createResponseBody의 로직이 일부와 중복이에요 !!

tomcat/src/main/java/nextstep/org/apache/coyote/http11/request/Http11Request.java

kdkdhoho · 2023-09-11T13:22:05Z

tomcat/src/main/java/nextstep/org/apache/coyote/http11/request/Http11Request.java

+    private StartLine startLine;
+    private Map<String, String> queryParams = new HashMap<>();
+    private Map<HttpHeader, String> headers = new HashMap<>();
+    private final Cookies cookies = new Cookies();


사실 쿠키도 헤더의 일종이니까 얘도 Header 내에서 처리할 수 있을 것 같아요!

이 부분은 set-cookie가 여러개가 한번에 반환될 수 있는 경우를 고려해서 구현한 부분이었습니다. 현재 headers는 key 값을 중복으로 가질 수 없기 때문에 여기에 "Set-Cookie" 헤더를 담는 경우 한 응답 내에 여러 "Set-Cookie" 헤더를 담을 수 없어서 이런 방식으로 구현했습니다 ㅎㅎ...

set-cookie가 한 번에 여러 개 반환되는 경우도 고려하셨군요! 엄청 꼼꼼하시네요 👍
그런데, Set-Cookie가 이미 Map에 있다면, Value를 이어 붙이는 방식으로 추가해나가도 될 것 같아요 👀

만약 스프링이었다면 MultiValueMap을 썼겠죠? 😅

MoonJeWoong

안녕하세요 도기! 히이로입니다.

리뷰해주신 부분들 수정 반영하고 리뷰 재요청드립니다.
리뷰해주신 내용들에 대한 제 생각들을 정리한 부분들이 몇 군데 있는데 이 부분 얘기 나눠보시면서 미션 마무리 해도 좋을 것 같습니다 ㅎㅎ 조금 시간에 쫓겨 리팩토링 한 부분도 없지 않아 있어서 석연찮은 부분이 있으면 말씀해주시면 좋겠습니다.

바쁜 와중에도 정성 넘치는 리뷰 해주셔서 많이 배웠습니다. 감사합니다!! 🙇

tomcat/src/main/java/nextstep/org/apache/coyote/http11/Http11Processor.java

tomcat/src/main/java/nextstep/org/apache/catalina/connector/Connector.java

MoonJeWoong · 2023-09-12T00:23:53Z

tomcat/src/main/java/nextstep/org/apache/catalina/Context.java

+import nextstep.org.apache.catalina.servlet.RegisterServlet;
+import nextstep.org.apache.catalina.servlet.Servlet;
+
+public class Context {


catalina 패키지 내에서 servlet context 역할을 수행하는 객체 클래스 네이밍을 context라고 사용하더라구요! 그래서 해당 부분을 반영해서 작성해본 네이밍이었습니다 ㅎㅎ...

tomcat/src/main/java/nextstep/org/apache/catalina/servlet/AbstractServlet.java

MoonJeWoong · 2023-09-12T09:13:28Z

tomcat/src/main/java/nextstep/org/apache/coyote/http11/request/StartLine.java

@@ -14,14 +14,14 @@ public class StartLine {

    private String httpMethod;
    private String path;
-    private String httpVersion;
    private String queryString = null;


저는 개인적으로 생각해봤을 때 path라는 변수에 쿼리스트링이 포함된 값이 저장되는게 어색하더라구요~ 혹시 Http정의에서 path는 queryString을 포함하는 개념일까요? 그런 부분이 있다면 다시 알려주시면 감사하겠습니당 🙇

MoonJeWoong · 2023-09-12T11:25:30Z

tomcat/src/main/java/nextstep/org/apache/coyote/http11/HttpUtil.java

 import java.util.Map;
+import java.util.Objects;
+import java.util.Optional;

 public class HttpUtil {


음 Http11Request 뿐만 아니라 Http11Processor를 비롯해 다른 서블릿에서도 사용될 수 있는 로직들이라 util로 분리해냈는데요, 어떤 부분에서 도기가 그렇게 생각하셨는지 좀 더 말씀해주셔도 좋을 것 같습니다 :)

tomcat/src/main/java/nextstep/org/apache/coyote/http11/request/Http11Request.java

MoonJeWoong · 2023-09-12T11:33:57Z

tomcat/src/main/java/nextstep/org/apache/coyote/http11/request/Http11Request.java

+    private StartLine startLine;
+    private Map<String, String> queryParams = new HashMap<>();
+    private Map<HttpHeader, String> headers = new HashMap<>();
+    private final Cookies cookies = new Cookies();


이 부분은 set-cookie가 여러개가 한번에 반환될 수 있는 경우를 고려해서 구현한 부분이었습니다. 현재 headers는 key 값을 중복으로 가질 수 없기 때문에 여기에 "Set-Cookie" 헤더를 담는 경우 한 응답 내에 여러 "Set-Cookie" 헤더를 담을 수 없어서 이런 방식으로 구현했습니다 ㅎㅎ...

tomcat/src/main/java/nextstep/org/apache/coyote/http11/request/Http11Request.java

sonarcloud · 2023-09-12T11:45:19Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
8 Code Smells

0.0% Coverage
0.0% Duplication

The version of Java (11.0.20.1) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

kdkdhoho · 2023-09-12T14:08:18Z

안녕하세요 히이로 !!
리팩터링도, 남겨주신 댓글도 모두 잘 확인했습니다.
남겨주신 댓글에 여러 질문이나 히이로의 생각을 다시 묻는 답글을 남겼는데요, 사실 바로 머지해도 될 것 같으나 히이로랑 티키타카하는 게 재밌어서 바로 머지는 안할래요 ㅎㅎ 그런데 혹여나 일정 상 버겁거나 부담스러우면 꼭 말씀해주세요 !!

시간 괜찮으실 때 편하게 남겨주세요. 아님 직접 만나서 이야기 나눠도 좋을 것 같아요.
그럼 죄송하지만 한 번 더 부탁드릴게요 🙏

MoonJeWoong added 15 commits September 7, 2023 22:03

refactor: httpRequest 객체 분리

5248e44

refactor: InputStream의 보조 스트림은 request 객체 내부로 캡슐화

e6cf263

refactor: httpRequest 에서 cookies 반환 시 방어적 복사 기능 구현

e9be59d

refactor: Http11Response 객체 분리

51291fa

refactor: HttpRequest 객체명 Http11Request로 수정

c6cf4b2

feat: 501 status enum 추가

6b3ed16

feat: Servlet 인터페이스 및 AbstractServlet 클래스 구현

e4c9fb5

feat: LoginServlet 기능 분리 및 구현

23c3c39

feat: RegisterServlet 기능 분리 및 구현

384b755

feat: 서블릿 컨테이너 Context 클래스 구현 및 DefaultServlet 구현

ae3ac59

feat: 응답 메세지 바디 생성 메서드 공통 Util 클래스로 분리

4aa7b93

style: catalina, coyote 패키지 역할 별 분리

24ec969

test: thread 관련 학습 테스트 진행

84bb261

feat: tomcat connector 쓰레드 풀 설정 추가

7f81abc

feat: 로그인 session 동시성 관리를 위한 ConcurrentHashMap 사용

7d922b6

MoonJeWoong requested a review from kdkdhoho September 10, 2023 15:56

feat: HttpHeader enum 작성

77de1e6

kdkdhoho requested changes Sep 11, 2023

View reviewed changes

MoonJeWoong added 7 commits September 12, 2023 09:12

feat: Connector 상수 분리

5016190

feat: Connector 쓰레드 풀 재설정

055e620

refactor: Servlet 메서드 순서 변경

881b66d

refactor: AbstractServlet 상수 분리

ad63ded

refactor: LoginServlet 객체 static 선언 추가

25a5754

refactor: LoginServlet 객체 내 SessionManager 전역으로 관리하도록 수정

8c99b09

refactor: context를 Tomcat 객체에서 생성 후 의존성 주입하는 방향으로 수정

681e7fb

MoonJeWoong commented Sep 12, 2023

View reviewed changes

refactor: startLine 객체 내 queryString을 queryParams로 수정

af65667

kdkdhoho merged commit aa584ed into woowacourse:moonjewoong Sep 13, 2023
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[톰캣 구현하기 - 3, 4단계] 히이로(문제웅) 미션 제출합니다. #455

[톰캣 구현하기 - 3, 4단계] 히이로(문제웅) 미션 제출합니다. #455

MoonJeWoong commented Sep 10, 2023 •

edited

Loading

kdkdhoho left a comment •

edited

Loading

kdkdhoho Sep 11, 2023

MoonJeWoong Sep 12, 2023

kdkdhoho Sep 12, 2023

kdkdhoho Sep 11, 2023

MoonJeWoong Sep 12, 2023

kdkdhoho Sep 12, 2023

kdkdhoho Sep 11, 2023

MoonJeWoong Sep 12, 2023

kdkdhoho Sep 12, 2023

MoonJeWoong left a comment

MoonJeWoong Sep 12, 2023

MoonJeWoong Sep 12, 2023

MoonJeWoong Sep 12, 2023

MoonJeWoong Sep 12, 2023

sonarcloud bot commented Sep 12, 2023

kdkdhoho commented Sep 12, 2023 •

edited

Loading

[톰캣 구현하기 - 3, 4단계] 히이로(문제웅) 미션 제출합니다. #455

[톰캣 구현하기 - 3, 4단계] 히이로(문제웅) 미션 제출합니다. #455

Conversation

MoonJeWoong commented Sep 10, 2023 • edited Loading

kdkdhoho left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MoonJeWoong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Sep 12, 2023

kdkdhoho commented Sep 12, 2023 • edited Loading

MoonJeWoong commented Sep 10, 2023 •

edited

Loading

kdkdhoho left a comment •

edited

Loading

kdkdhoho commented Sep 12, 2023 •

edited

Loading